Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade graphql-upload to support Node.js v14 #4039

Closed
wants to merge 1 commit into from

Conversation

Leko
Copy link
Contributor

@Leko Leko commented Apr 27, 2020

Hi there!
I've updated graphql-upload to v10 to support Node.js v14. The documentation(docs/source/data/file-uploads.md) has already been updated to call createReadStream, but test codes ware still accessing the stream property, so I've updated the test codes.

fix #3508 and fix #3579

@apollo-cla
Copy link

@Leko: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/

@abernix
Copy link
Member

abernix commented Apr 28, 2020

Thanks for opening this! Are there any other breaking changes to the exposed API (e.g. the uploadsConfig configuration object) that end users need to be concerned with?

In other words, is this okay to introduce in a 2.x release?

@abernix abernix self-assigned this Apr 28, 2020
@Leko
Copy link
Contributor Author

Leko commented Apr 28, 2020

Hi @abernix!
uploadsConfig (FileUploadOptions) hasn't changed between v8 and v10 (documentation).

In other words, is this okay to introduce in a 2.x release?

I think It's OK for me but not for all end users. Especially, end users need to migrate to file.createReadStream() from file.stream. If users are already doing so, there is no breaking change.
Does my answer make sense?

@abernix abernix added this to the Release 3.x milestone Apr 29, 2020
@abernix abernix added the 🍳 breaking-change Needs to wait for a major release. label Apr 29, 2020
@abernix
Copy link
Member

abernix commented Apr 29, 2020

It does make sense, but it sounds like we'd still need to put this in a major release to me. Thanks for the clarity.

@Vultraz
Copy link

Vultraz commented May 6, 2020

Would this be in 3.x, then?

@jaydenseric
Copy link

Note that graphql-upload v11.0.0 is out.

@savnik
Copy link

savnik commented May 27, 2020

Updating will also solve this issue/bug #3508 with fs-capacitor. In our case we can remove the forced resolution of version 5.0.0 in our package.json files.

@@ -41,7 +41,7 @@
"graphql-extensions": "file:../graphql-extensions",
"graphql-tag": "^2.9.2",
"graphql-tools": "^4.0.0",
"graphql-upload": "^8.0.2",
"graphql-upload": "^10.0.0",
Copy link

@savnik savnik May 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update this to version 11.0.0 as noted by @jaydenseric.
One of the changes in version 11.0.0 is that graphql-upload is actaually CI tested for node version 14.
Version 10.0.0 of graphql-upload is not CI tested for node version 14. See release notes here: https://github.com/jaydenseric/graphql-upload/releases

@standy
Copy link

standy commented Jun 9, 2020

If I'm not wrong, the next major release is only 30% complete and doesn't have a date.

This fix is pretty crucial and should be released sooner is better, because for now apollo-server doesn't work with current nodejs versions properly

@gengjiawen
Copy link
Contributor

If I'm not wrong, the next major release is only 30% complete and doesn't have a date.

This fix is pretty crucial and should be released sooner is better, because for now apollo-server doesn't work with current nodejs versions properly

@abernix Anything you can do to push this a little forward ?

@abenhamdine
Copy link

abenhamdine commented Jun 19, 2020

@Leko could you update your PR to change grapqhl-upload version to 11 ?
It's currently blocking the approval

thx !

@Leko Leko force-pushed the support-nodejs14 branch from 9253f2e to e430769 Compare June 19, 2020 13:10
@Leko
Copy link
Contributor Author

Leko commented Jun 19, 2020

@abenhamdine I'm sorry for my late reply. I've updated graphql-upload to 11.x.

Copy link

@savnik savnik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@abernix abernix closed this Jun 24, 2020
@Vultraz
Copy link

Vultraz commented Jun 25, 2020

Did this get handled elsewhere?

@abenhamdine
Copy link

Did this get handled elsewhere?

I don't think, see #4304

@abernix abernix reopened this Jun 25, 2020
@abernix abernix changed the base branch from master to main June 25, 2020 09:02
@strigona-worksight
Copy link

Any updates on when this might be merged?

@Noloxe-Fred
Copy link

Any updates on when this might be merged?

Same question ;)

@abernix
Copy link
Member

abernix commented Jul 22, 2020

To those that are inquiring, I posted a suggestion / update on the issue here: #3508 (comment)

@JCMais
Copy link

JCMais commented Jul 31, 2020

For those that cannot wait and are receiving TypeError: _graphql.GraphQLScalarType is not a constructor on Node.js 14, add this to the resolutions on your package.json (this only works if you use Yarn):

    "**/apollo-server-core/graphql-upload": "^11.0.0",

@sakulstra
Copy link

sakulstra commented Aug 9, 2020

It does make sense, but it sounds like we'd still need to put this in a major release to me. Thanks for the clarity.

Couldn't we do a major release with this and rebrand the other upcoming changes to v4?
Seems like apollo-server has fallen behind in some dependencies (^this gql-upload, but also graphql, apollo-tools et al) resulting in quite some resolution work to not break our toolchain 😅

@woowebsite
Copy link

woowebsite commented Dec 10, 2020

after upgrade fs-capacitor in apollo-server-core/node_modules/fs-capacitor to version 6.2.0 I have resolved.
I use Nodejs 14

@alvis
Copy link

alvis commented Dec 27, 2020

On node 15, npm v7 even gives a warning when installing apollo-server as graphql-upload v8 doesn't support graphql v15. :/

@simontaisne
Copy link

Is there any reason for not going ahead with this PR?

@savnik
Copy link

savnik commented Feb 1, 2021

@simontaisne I think that the point form @abernix is to put it in the next major release because of the breaking changes.
But it could be nice with an update :-)

@glasser
Copy link
Member

glasser commented May 2, 2021

As described in #3508 (comment) we will be removing the default graphql-upload integration in Apollo Server 3, so we won't be upgrading it. @abernix has commented above with instructions on how to use graphql-upload directly in Apollo Server 2.

@glasser glasser closed this May 2, 2021
@glasser glasser removed this from the Release 3.x milestone May 2, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
🍳 breaking-change Needs to wait for a major release.
Projects
None yet